Skip to content

fix(node): bound the emitted Arrow IPC stream length at the producer - #2787

Closed
phil-opp wants to merge 1 commit into
mainfrom
claude/clever-wright-tzo5ua-ipc-size-guard
Closed

fix(node): bound the emitted Arrow IPC stream length at the producer#2787
phil-opp wants to merge 1 commit into
mainfrom
claude/clever-wright-tzo5ua-ipc-size-guard

Conversation

@phil-opp

Copy link
Copy Markdown
Collaborator

Summary

Fixes #2586.

The Arrow IPC data plane (#2366) guarded the input length against MAX_IPC_BYTES at encode time, but the emitted IPC stream is ~1.125× larger (validity bitmap data_len/8, 64-byte alignment padding, and the schema + record-batch message blocks). Every receiver bounds the whole stream against the same MAX_IPC_BYTES. So a payload whose input length sits just under the cap (roughly (227.5 MiB, 256 MiB] for UInt8) could pass the encode guard and be sent, then be rejected by all receivers — a silent drop on the zenoh receive path.

Changes

Guard the resulting stream length instead, so anything the producer accepts is decodable by every receiver (or it fails loudly at the producer):

  • uint8_layout (ipc_encode.rs): now bounds the computed total stream length, not just data_len. Covers the send_output_raw UInt8 construct-in-place path — including the Python bindings (apis/python/node/.../sample_handler.rs) and send_output_bytes (mod.rs), both of which go through uint8_ipc_len / encode_uint8_ipc_header. The pre-existing data_len > MAX_IPC_BYTES check is kept as an overflow-safety bound before total is computed.
  • send_output_array (mod.rs): runs check_ipc_size on the fast-path length and on the fallback stream length before allocating the sample, so the general array path (both the zero-copy fast path and the official-writer fallback) also fails loudly rather than emitting an undecodable stream.
  • check_ipc_size is promoted to pub(crate) and its doc updated to note the dual producer/decode role.

Tests

Added uint8_ipc_len_rejects_oversized_stream: data_len == MAX_IPC_BYTES clears a naive data_len check but the stream overhead pushes the emitted stream over the cap, so it must be rejected; a small payload still encodes to an in-bounds stream. Full ipc_encode suite (33 tests) passes; cargo fmt --check and cargo clippy -p dora-node-api -- -D warnings are clean.

Notes

The receiver-side guards are unchanged — this only tightens the producer side so the encode and decode limits agree.

🤖 Generated with Claude Code


Generated by Claude Code

@trunk-io

trunk-io Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Merging to main in this repository is managed by Trunk.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here

Copy link
Copy Markdown
Collaborator Author

🤖 Automated review by Claude — this is a fully automated review with no human in the loop; treat findings as suggestions to verify, not as authority.

No issues found.

The fix correctly guards the emitted stream length rather than the input data_len: uint8_layout now bails when the computed total exceeds MAX_IPC_BYTES, and send_output_array runs the now-shared check_ipc_size on both the fast-path length and the fallback stream before allocating. This closes the window where a payload just under the cap encoded to an over-cap stream that every receiver silently dropped on the zenoh path — the producer now fails loudly instead. The producer and decode guards share one constant, so the encode/decode limits cannot drift.


Generated by Claude Code

Copy link
Copy Markdown
Collaborator Author

🤖 Automated review by Claude — fully automated, no human in the loop; it may contain mistakes.

The bound logic itself looks correct — it bounds the actual emitted total against the shared MAX_IPC_BYTES, with no overflow given the data_len pre-filter. But the same fix for #2586 already landed on main as #2588 (53a0edf), which adds the identical total > MAX_IPC_BYTES bail in uint8_layout and also guards the generic fast path in prepare() (returns None → routes to the fallback encoder, which bails).

Relative to current main, the only novel piece here is the two check_ipc_size calls in send_output_array, and those look redundant now:

  • the Some(len) fast-path branch is unreachable for oversized streams, since ipc_fast_path_lenprepare already returns None when total > MAX_IPC_BYTES; and
  • in the None branch, encode_ipc_to_vecencode_arrow_ipc already bails on buf.len() > MAX_IPC_BYTES before the added check runs.

The PR also currently shows as conflicting with main. If #2588 covers the intended cases, this one is probably safe to close.


Generated by Claude Code

@phil-opp
phil-opp force-pushed the claude/clever-wright-tzo5ua-ipc-size-guard branch from 874cc7e to cca4a51 Compare July 28, 2026 18:07

Copy link
Copy Markdown
Collaborator Author

🤖 Automated review by Claude — this is a fully automated review, with no human in the loop verifying the findings below.

Re-reviewed after the rebase onto main. The conflict is resolved and the uint8_layout hunk (identical to #2588) is gone from the diff, but the substance is unchanged and the redundancy noted earlier still stands — this PR adds no behavioral change over current main (post-#2588 / 53a0edf):

  • Both check_ipc_size calls added in send_output_array (apis/rust/node/src/node/mod.rs) are unreachable:
    • Some(len) branch — ipc_fast_path_lenprepare() already returns None when total > MAX_IPC_BYTES (arrow_utils/ipc_encode.rs:318), so len <= MAX_IPC_BYTES is guaranteed here; the check can never fire.
    • None branch — encode_ipc_to_vecencode_arrow_ipc already bails on buf.len() > MAX_IPC_BYTES (arrow_utils.rs:63) before returning Ok; the check on bytes.len() can never fire.
  • The new test uint8_ipc_len_rejects_oversized_stream duplicates the existing uint8_ipc_len_bounds_resulting_stream_not_input from fix(node): bound the resulting Arrow IPC stream length in encode guards #2588 (arrow_utils/ipc_encode.rs:1294), which already asserts uint8_ipc_len(MAX_IPC_BYTES) is rejected.

The bound arithmetic itself is correct, but the producer-side guard #2586 asked for is already provided on the live send path by #2588. Unless there's an intended case #2588 doesn't cover, this looks safe to close.


Generated by Claude Code

@phil-opp
phil-opp marked this pull request as ready for review August 9, 2026 11:33
The Arrow IPC encode guards bounded the *input* length (`data_len` in
`uint8_layout`, the body offset in the fast-path builder) against
`MAX_IPC_BYTES`, but the emitted stream is ~1.125x larger (validity
bitmap, 64-byte alignment padding, schema + record-batch message blocks).
Every receiver bounds the *whole stream* against the same `MAX_IPC_BYTES`,
so a payload whose `data_len` sits just under the cap could encode and be
sent, then be rejected by all receivers — a silent drop on the zenoh path.

Guard the resulting stream length instead, failing loudly at the producer:

- `uint8_layout` now checks the computed `total` (covers the
  `send_output_raw` UInt8 construct-in-place path, incl. the Python
  bindings).
- `send_output_array` runs `check_ipc_size` on the fast-path length and on
  the fallback stream before allocating, so anything the producer emits is
  decodable by every receiver.

`check_ipc_size` is now shared (producer + decode guard); its doc notes the
dual role.

Fixes #2586

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01C79MriEbXiurvZQQ3QoCkU
@phil-opp
phil-opp force-pushed the claude/clever-wright-tzo5ua-ipc-size-guard branch from cca4a51 to 9c7e141 Compare August 11, 2026 16:07

Copy link
Copy Markdown
Collaborator Author

🤖 Fully automated review by Claude — this review was generated end-to-end by an automated agent with no human vetting. Treat it accordingly.

Re-checked after the rebase that moved the two check_ipc_size calls from send_output_array into SampleAllocator::encode_arrow (apis/rust/node/src/node/mod.rs:2712). The earlier observation still holds at the new location — both calls are unreachable:

  • Some(prepared) branch (mod.rs:2716): PreparedIpc::new is prepare(array).map(Self), and prepare() returns None whenever total > MAX_IPC_BYTES (arrow_utils/ipc_encode.rs:318), so prepared.byte_len() is always <= MAX_IPC_BYTES here; the check can't fire.
  • None branch (mod.rs:2731): encode_ipc_to_vecencode_arrow_ipc already bails on buf.len() > MAX_IPC_BYTES (arrow_utils.rs:81) and returns Err before the added check_ipc_size(bytes.len()) runs.

The uint8_layout total guard that #2586 actually needs is already on main (#2588), and uint8_ipc_len_rejects_oversized_stream overlaps the existing uint8_ipc_len_bounds_resulting_stream_not_input (it exercises uint8_layout, not the two lines this PR adds). So this still looks like it has no behavioral delta over current main — I don't see why the change is necessary unless there's a path #2588 doesn't cover.


Generated by Claude Code

@phil-opp

Copy link
Copy Markdown
Collaborator Author

Closing as already fixed. The producer-side bound landed via #2588: prepare() computes the real serialized total and returns None above MAX_IPC_BYTES, uint8_layout carries both the pre-filter and the total bail, and encode_arrow_ipc bails on oversize. Both check_ipc_size calls in this branch are unreachable as a result, and the added test is a strict subset of one already on main.

The branch is also no longer rebaseable as-is: send_output_array was refactored out of existence, so the hunk has no landing site.

@phil-opp phil-opp closed this Aug 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Arrow IPC encode/decode size guards are asymmetric: large UInt8 payloads encode but are rejected on receive (silent drop)

2 participants